Repository navigation
[libc++] Use Fuchsia futex operations - #133571
Conversation
llvmbot
added
the
libc++
|
@llvm/pr-subscribers-libcxx Author: Roland McGrath (frobtech) ChangesThe basic futex operations have always been available on Fuchsia. Full diff: https://github.com/llvm/llvm-project/pull/133571.diff 3 Files Affected:
diff --git a/libcxx/include/__atomic/contention_t.h b/libcxx/include/__atomic/contention_t.h
index 5b42a0125f875..cd3a8e4b29094 100644
--- a/libcxx/include/__atomic/contention_t.h
+++ b/libcxx/include/__atomic/contention_t.h
@@ -19,7 +19,7 @@
_LIBCPP_BEGIN_NAMESPACE_STD
-#if defined(__linux__) || (defined(_AIX) && !defined(__64BIT__))
+#if defined(__linux__) || defined(__Fuchsia__) || (defined(_AIX) && !defined(__64BIT__))
using __cxx_contention_t _LIBCPP_NODEBUG = int32_t;
#else
using __cxx_contention_t _LIBCPP_NODEBUG = int64_t;
diff --git a/libcxx/include/__cxx03/__atomic/contention_t.h b/libcxx/include/__cxx03/__atomic/contention_t.h
index a97f0668da2fe..3fc3ccef78df5 100644
--- a/libcxx/include/__cxx03/__atomic/contention_t.h
+++ b/libcxx/include/__cxx03/__atomic/contention_t.h
@@ -19,7 +19,7 @@
_LIBCPP_BEGIN_NAMESPACE_STD
-#if defined(__linux__) || (defined(_AIX) && !defined(__64BIT__))
+#if defined(__linux__) || defined(__Fuchsia__) || (defined(_AIX) && !defined(__64BIT__))
using __cxx_contention_t = int32_t;
#else
using __cxx_contention_t = int64_t;
diff --git a/libcxx/src/atomic.cpp b/libcxx/src/atomic.cpp
index c1af8d6f95aae..6de355b534b08 100644
--- a/libcxx/src/atomic.cpp
+++ b/libcxx/src/atomic.cpp
@@ -41,6 +41,10 @@
// OpenBSD has no indirect syscalls
# define _LIBCPP_FUTEX(...) futex(__VA_ARGS__)
+#elif defined(__Fuchsia__)
+
+# include <zircon/syscalls.h>
+
#else // <- Add other operating systems here
// Baseline needs no new headers
@@ -101,6 +105,24 @@ static void __libcpp_platform_wake_by_address(__cxx_atomic_contention_t const vo
_umtx_op(const_cast<__cxx_atomic_contention_t*>(__ptr), UMTX_OP_WAKE, __notify_one ? 1 : INT_MAX, nullptr, nullptr);
}
+#elif defined(__Fuchsia__)
+
+static inline zx_futex_t const* __libcpp_zx_futex(__cxx_atomic_contention_t const volatile* ptr) {
+ // Implicitly link against the vDSO system call ABI without requiring the
+ // final link to specify -lzircon explicitly when statically linking libc++.
+# pragma comment(lib, "zircon")
+ return const_cast<zx_futex_t const*>(reinterpret_cast<zx_futex_t const volatile*>(&ptr->__a_value));
+}
+
+static void
+__libcpp_platform_wait_on_address(__cxx_atomic_contention_t const volatile* __ptr, __cxx_contention_t __val) {
+ _zx_futex_wait(__libcpp_zx_futex(__ptr), __val, ZX_HANDLE_INVALID, ZX_TIME_INFINITE);
+}
+
+static void __libcpp_platform_wake_by_address(__cxx_atomic_contention_t const volatile* __ptr, bool __notify_one) {
+ _zx_futex_wake(__libcpp_zx_futex(__ptr), __notify_one ? 1 : UINT32_MAX);
+}
+
#else // <- Add other operating systems here
// Baseline is just a timed backoff
|
@
@petrhosek let me know your plan. There will be some merge conflicts. My patch basically allow more types to going directly to the platform api under the unstable ABI |
|
I've updated after all the internal refactorings of past months, and incorporated other review comments. I think it's looking pretty clean now. Please take a fresh look! Thanks. |
huixie90
left a comment
huixie90
left a comment
There was a problem hiding this comment.
LGTM with some clarification
| // final link to specify -lzircon explicitly when statically linking libc++. | ||
| # pragma comment(lib, "zircon") | ||
|
|
||
| return reinterpret_cast<zx_futex_t const*>(__ptr); |
There was a problem hiding this comment.
just wanted to bring you attention of
libcxx/include/__atomic/atomic_waitable_traits.h
and here is the design intent of how things work.
atomic_waitable_traits.h defines a concept __has_native_atomic_wait that decides which types can directly call the platform futex wait, and which types needs to go through the global contention table proxy for platform wait.
And there are two ABI modes.
For the default ABI, __has_native_atomic_wait basically says only the __cxx_contention_t satisfies, which is int32_t in your case, can directly go to platform futex wait. I assume here zx_futex_t is exactly int32_t otherwise we might have UB
For another ABI which has _LIBCPP_ABI_ATOMIC_WAIT_NATIVE_BY_SIZE defined, which is controlled by the platform owner. in this case, i believe your platform does not enable it. but say, one day you decide to enable this ABI, the way it works is that. you can define __has_native_atomic_wait the way you want, the default concept in this mode is: any trivially copyable type with unique value representations, whose sizes matches __cxx_contention_t. in this case, __ptr and __val can be pointers to a wide range of types, e.g. a uint32_t, struct S { int32_t i;}; etc... In which case, I am not sure how reinterpret_cast<zx_futex_t const*> can be well defined. (anyway you don't need to worry about this if you don't plan to enable this ABI)
There was a problem hiding this comment.
We do have _LIBCPP_ABI_ATOMIC_WAIT_NATIVE_BY_SIZE enabled by dint of the fact that we always use ABI version 2. I'd be happy for it to be unconditionally enabled for Fuchsia, but I didn't see any precedent for that setting being controlled by anything but _LIBCPP_ABI_VERSION. In what sense is it controlled by the platform owner?
AFAICT there is no change to __atomic/atomic_waitable_traits.h required because __cxx_contention_t is already always int32_t (as my changes here to __atomic/contention_t.h) so the default case uses sizeof(__cxx_contention_t). For the same reason, the #ifdef __linux__ branch there seems superfluous.
I'm not sure what the concern is about the reinterpret_cast. zx_futex_t is just an integer type with the same size as int32_t, so it's layout compatible with any integer type or struct of integer type of that size.
There was a problem hiding this comment.
In what sense is it controlled by the platform owner?
Vendors can configure those individual ABI macros in the __config_site. See the doc here
https://libcxx.llvm.org/ABIGuarantees.html#abi-flags
And yes if you opt in _LIBCPP_ABI_VERSION to 2, you get all of these individual macros.
AFAICT there is no change to __atomic/atomic_waitable_traits.h required because __cxx_contention_t is already always int32_t
I think in your case, yes no action required. I mentioned that because some platforms can support multiple sizes for the platform wait. For example, Apple's ulock wait support both 4 bytes and 8 bytes wait.
I'm not sure what the concern is about the
reinterpret_cast
I think it is kind of "it works in practice" but I think it is a language UB if you reinterpret_cast from memory location when there is actually no object of that type has started life time (implicitly or explicitly)
There was a problem hiding this comment.
I see, so by "platform owner" you meant "person who sets the libc++ build parameters", not the owner of a particular platform's platform-specific code in libc++ sources. These are not always the same, and for Fuchsia it varies where I am both of those people and where I am only the latter. :-)
There is no language-based access here. The cast is just getting the pointer value to a system call, so regardless of the complexities of trivial object lifetime that might actually apply here, it's really outside the scope of those language rules anyway.
Thanks for the explanations! I think this change is good to go as is now. It just needs the approval from @philnik777 to be mergeable.
Dismissing because the reviewer said it was fine to modify the 03 headers now -- so his original concern was resolved.
The basic futex operations have always been available on Fuchsia. Wire them up to properly support C++20 atomic notify_*/wait.
The basic futex operations have always been available on Fuchsia.
Wire them up to properly support C++20 atomic notify_*/wait.